Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding --with for CTE #705

Merged
merged 2 commits into from
Jul 11, 2023
Merged

Adding --with for CTE #705

merged 2 commits into from
Jul 11, 2023

Conversation

neelasha23
Copy link

@neelasha23 neelasha23 commented Jul 3, 2023

Describe your changes

Adding --with again

Issue number

Closes #684

Checklist before requesting a review


📚 Documentation preview 📚: https://jupysql--705.org.readthedocs.build/en/705/

@neelasha23 neelasha23 marked this pull request as draft July 3, 2023 06:16
@neelasha23 neelasha23 marked this pull request as ready for review July 4, 2023 16:01
@neelasha23
Copy link
Author

@edublancas Please review

Copy link

@yafimvo yafimvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments.

@edublancas @neelasha23
General thought about the error message:
image

this part: If using CTEs, you may pass the --with argument explicitly.

  1. What do you think if we add a link to our --with documentation
  2. Explain in our --with docs why and when we should use --with (maybe to show this exact case with the error and the solution?)

src/sql/error_message.py Outdated Show resolved Hide resolved
src/sql/error_message.py Outdated Show resolved Hide resolved
src/sql/magic.py Outdated Show resolved Hide resolved
src/sql/magic.py Outdated Show resolved Hide resolved
src/sql/magic.py Outdated Show resolved Hide resolved
src/sql/magic.py Outdated Show resolved Hide resolved
src/sql/magic_plot.py Outdated Show resolved Hide resolved
src/tests/integration/test_generic_db_operations.py Outdated Show resolved Hide resolved
@neelasha23
Copy link
Author

  1. What do you think if we add a link to our --with documentation
  2. Explain in our --with docs why and when we should use --with (maybe to show this exact case with the error and the solution?)

Added these @yafimvo @edublancas

Copy link

@edublancas edublancas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added my feedback in a notebook: ctes.ipynb.zip

@neelasha23
Copy link
Author

A few doubts from the notebook:

Screenshot 2023-07-08 at 8 00 49 AM `This is now complaining about first_cte.` : I'm not sure which error? I don't see anything in this cell. Also, this would complain about `first_cte` since sqlglot wouldn't be able to parse the query? Screenshot 2023-07-08 at 8 01 14 AM This should work if we pass `--with` while saving the `second_cte` like: Screenshot 2023-07-08 at 8 07 41 AM

@edublancas

@edublancas
Copy link

This is now complaining about first_cte. : I'm not sure which error?

I'm referring to the cell below the "this is now complaining"

I don't see anything in this cell. Also, this would complain about first_cte since sqlglot wouldn't be able to parse the query?

Ah, you're right. there's no way around it and users will have to pass--with first_cte --with second_cte

then the only missing thing is improving the error message.

Copy link

@yafimvo yafimvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I guess what's left is the error message like @edublancas mentioned.

@@ -83,7 +83,7 @@ The percent format is also supported by `PyCharm Professional`:

![pycharm](../static/pycharm-interactive.png)

[Click here](https://jupytext.readthedocs.io/en/latest/formats.html#the-percent-format) for more details on the percent format.
[Click here](https://jupytext.readthedocs.io/en/latest/formats-scripts.html#the-percent-format) for more details on the percent format.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a new PR for this so we can fix our CI quickly. After we merge it, you can rebase.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neelasha23
You can rebase

@neelasha23
Copy link
Author

neelasha23 commented Jul 10, 2023

then the only missing thing is improving the error message.

I'm not clear on this part. Are u suggesting to remove the below specific error message completely? This was added as part of this issue
Screenshot 2023-07-10 at 5 33 30 PM

@edublancas

Lint

exception

Removed deprecation tests

Removed generic exception

condition removed

duckdb exception

duckdb exception

reraise exception

integration tests

fix

Integration tests

Integration tests

error message

assert usageerror

error msg

Error added

oracle

integration tests

docs

replaced numbers table

tests

tests

string formats

fix tests

display module for print

changed to print

test fix

docs

doc link

plot docs

plot docs

refactor plots

import moved

lint

error message

added cte msg

removed syntax error
@neelasha23
Copy link
Author

Have modified the error message. However, I still see the old message in docs.
I have cleared cache.

In local build I see the new one:

Screenshot 2023-07-11 at 7 11 42 PM

Do you also see the old one? @edublancas

@edublancas edublancas merged commit 27bef63 into ploomber:master Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to run DuckDB queries using FILTER clause on aggregates
4 participants